Phase 2: format completeness - #54
Conversation
Sector checksums (P2-3) are Adler-32 seeded with zero, not CRC32 and not standard Adler-32. Both StormLib call sites pass a seed of 0, which starts the accumulators one below where java.util.zip.Adler32 starts them; the results differ by 1 in the low half and by the byte count in the high half for every input. tools/mpqref.py now computes the value independently and caught the first attempt getting it wrong -- a reader and writer that both use the standard seed agree with each other and with nothing else. The checksum chunk is never encrypted and is zlib compressed. Fixing the read path meant fixing the verbatim-copy paths in both the core and the deprecated MpqFile, which decrypted every offset-table gap including that chunk; since a copy clears the encryption flags while keeping SECTOR_CRC, that would have written the corruption back as authoritative. (attributes) (P2-4, #11) is now modelled on its own bytemask rather than an assumed CRC32-plus-FILETIME layout, so a file carrying MD5 digests or patch bits is read instead of misread, and the unexplained -1 in the entry count is gone. Generation is opt-in with a pinnable timestamp. v2-v4 reading (P2-2) gains the hi-block table, compressed hash and block tables, and the version 3 MD5 digests -- reported through MpqArchive.integrity() rather than enforced, as StormLib does. HET/BET is split out as P2-2a and deliberately not attempted: it is not needed to extract StormLib-generated archives, and there is no fixture to verify it against. Also: the user data header is modelled instead of discarded (P2-1), and the header scan no longer commits to an implausible candidate (P2-5b, #47).
A file copied verbatim keeps the source's flags, so asking for checksums quietly meant "on whichever files happened to be re-encoded anyway" and produced a half-checksummed archive. Checksums are computed per stored sector, so adding them requires re-encoding; a file that already carries them can still be copied.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6489f56502
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Every version 3 header carries all six MD5 fields, so their presence says nothing about whether a digest was recorded. hasDigests() looked at the lengths, which are always 16, so an archive that left the fields blank reported VERIFIED -- agreement with digests nobody computed. It now applies the same all-zero convention matchesDigest already used. Integrity.VERIFIED promises every recorded digest matched, so it has to cover the HET and BET tables too. This library does not read those tables but it can check their digests, and skipping them let an archive whose HET table was the damaged one report clean. The patch-bit array turns out to have two legal lengths, because StormLib disagrees with itself: GetSizeOfAttributesFile sizes it (n + 6) / 8 while the loader sizes it (n + 7) / 8. Those differ when n is congruent to 1 modulo 8 -- a one-block archive is allotted zero bytes for one bit. Parsing now accepts either length, bits the file does not reach read as unset instead of out of bounds, and what we emit is the length that holds every bit so a write cannot leave the buffer. Version3IntegrityTests builds a version 3 archive by relocating a version 1 one, which also gives the v2-v4 read path its first end-to-end coverage.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9740074d01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The header scan screened candidates on room for the uncompressed hash table, which rules out a valid version 3 archive whose compressed table sits at the end of the file -- and once such a header is rejected, a decoy planted earlier wins the scan, which is the reverse of what the check is for. This was a regression introduced with the scan change earlier in this branch. The stored length is now used where the header declares one. A recorded digest whose region is absent, empty or outside the file counted as a match, so VERIFIED could be reported with nothing hashed. Since the HET and BET tables are not otherwise read or validated, that path was the only thing between a damaged extended table and a clean report. It is now a mismatch, and the table-absent case is passed in explicitly rather than inferred from a zero position. The fixtures grew a genuinely compressed hash table on the way, which compressed tables had no coverage of at all before now.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83daf9b444
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A decoy with in-range table positions but a format version above MAX_FORMAT_VERSION passed every part of the plausibility test, so the scan committed to it and then failed in parseAt as unsupported -- with a valid header possibly still ahead of it in the file. Same class as the previous finding: the screen was not rejecting headers that cannot be used. The version screen is skipped under forceV0, which exists precisely to read archives whose declared version is garbage. For the same reason the compressed hash-table size is only consulted for a candidate that will actually be read as version 3; under forceV0 those bytes mean nothing, and the check now says version 3 rather than version 3 or above. Both directions are covered, and I checked the decoy test fails when the screen is removed. Format notes gain section 14 on what the scan requires and why strengthening it needs care.
|
@codex review |
Two review rounds went on this method one condition at a time, both times because the screen accepted a candidate parseAt would refuse. So the rest are done together: the hash table entry maximum and, for a version 3 candidate, a compressed block table that runs past the end. The invariant is now written down where it belongs. isPlausible must reject only what parseAt rejects -- rejecting more is not a stricter filter but a bug, because the scan then moves on and can settle on an earlier decoy. Everything the parser merely repairs (a wrong header size, an oversized block table, an out-of-range hi-block position) is deliberately not screened, and there is now a test asserting that too, so tightening this again cannot quietly start losing readable archives. One screen is documented as unable to change the outcome today: a candidate without room for its own header is necessarily the last one in the file, so no later header exists to reach. It is kept to make the screen a faithful mirror rather than a set of conditions that happen to matter, and the test says why it is not covered.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f4d75790
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rsions The block-table range check validated against a header model still holding the raw negative count, so its byte length came out negative, the check always failed, and the recovery path replaced the clamped zero with every 16-byte row between the table and EOF -- reading trailing bytes as live block entries. The half-built model is gone; the check now uses the clamped count, and the compressed-or-plain rule has one implementation instead of being spelled out in three places. An (attributes) file declaring a version other than 100 was parsed with the version 100 layout, so a same-length future or corrupt file produced plausible-looking checksums and timestamps describing nothing. 100 is the only version the format has had, so anything else is now reported. The archive still opens either way -- attributes are advisory. Both verified to fail with the fix backed out. Full suite: 195 tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 160ee8aa2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The plausibility screen read low-only table offsets while the parser combines them with the high words at 0x28/0x2A from version 1 onward. So a decoy with in-range low offsets and a non-zero high word passed the screen and then failed to parse, with a valid header still ahead of it in the file. This is the same mismatch the previous two rounds were about, and the screen's own doc comment claimed to mirror the parser exactly -- it did for the thresholds and not for the offsets. Asserting an invariant in a comment did not keep it true, so the offsets now come from one tablePosition helper that both sides call, which is the same treatment storedSize got. The version-3 size helpers take the resolved version instead of re-deriving it, removing the last two places that decided independently what version a candidate is. Covered both ways round: a high word on either table offset, verified to fail with the low-only reads restored.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 678d99f1d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
MPQ names are case-insensitive, but the writer filtered its carry-over list by exact string. A source whose listfile spells its attributes file (ATTRIBUTES) therefore had the stale file carried into pending, where the collision check -- which does canonicalise -- then refused to build at all. So rebuilding such an archive with attributes enabled failed outright. Three sites decided internal-name identity three different ways: exact match here, equalsIgnoreCase for the generated list file, and exact match again when counting what a rebuild drops. All three now go through MpqNames.canonical, the same fold the hash table and the writer's own keys use, so they cannot disagree. This is the same mistake as comparing paths without folding their separators, which cost a file on rebuild in Phase 0.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0155098a5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bounds that cannot locate the checksum chunk -- negative, reversed, or past the stored bytes -- were treated the same as an empty chunk, so default verification returned the file without checking it. The data sectors are delimited by the same offset table, so if its last entries are nonsense the sector entries are only accidentally still in range; handing the bytes back is the one outcome verification must not produce. Same principle as the digest round: unable to check is not the same as checked. An empty chunk stays legitimate, since a file may carry the flag and record nothing. The sibling case is fixed too: a chunk too short to hold one checksum per sector is corrupt rather than absent, and was also being swallowed. Recovering a damaged archive is still possible through withSectorChecksumVerification(false), which is what makes failing the default read the right default.
|
@codex review |
The hand-written Adler-32 loop used int accumulators with zlib's NMAX fold interval. zlib picks that interval so s2 cannot overflow an *unsigned* 32-bit accumulator; Java has no such type and a signed int overflows at half that, so a sector of a few thousand high-valued bytes was checksummed wrongly. Reachable in practice: the default recompression setting never shrinks a sector, so raw bytes reach the checksum as they are. Now computed with java.util.zip.Adler32 and corrected for the seed, which differs by a closed form -- 1 in the low half, one per byte in the high half. That is also the faster path, since Adler32.update is a HotSpot intrinsic and a Java loop is not, and sectors can be 16 MiB. The bug survived a round trip, a green suite and a review for the same reason the seed itself did: reader and writer shared the wrong arithmetic and agreed with each other. It took an independent implementation plus a fixture with high-valued stored sectors, which is now exported for CI. The old loop survives as the oracle the fast path is fuzzed against, with long accumulators so it is actually correct. Also aligns the hi-block table with StormLib: an archive declaring one it cannot hold is reported rather than read with the low words alone, which silently relocated every file. FORCE_V0 still opens such an archive, since a version 0 header has no such field.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8325be11d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…load The parser claimed to read the known prefix of an attributes file and ignore the rest, and did not: it required the length to match the known arrays exactly, so any file actually carrying an unknown array was rejected and its perfectly good checksums went with it. An exact length is still preferred; a longer file is accepted only when the bytemask names an array this implementation does not know, so a tail nothing explains stays an error. The test that was supposed to cover this passed for the wrong reason: it set an unknown flag without appending the array it names, so the length still matched exactly and the case was never exercised. It now appends the array. A user data header declaring more payload than fits before the archive returned the archive itself as metadata, because the clamp was to the end of the file rather than to the redirect offset -- which is the actual end of the user data area. The narrowing to an array length is bounded now too, since both clamps are 64-bit. MpqUserData.payload needs an MpqSource, which no caller outside this package can obtain, so it was unreachable. MpqArchive.userDataPayload is the way in.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c200e32ed2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Neither size field in a user data header is the payload length. StormLib
documents cbUserDataSize as the *maximum* size of the area -- a capacity --
and its comment on cbUserDataHeader is openly unsure ("Appears to be size
of user data header"). What SFileGetFileInfo actually returns for
SFileMpqUserData is the span between the two headers:
ha->UserDataPos + sizeof(TMPQUserData),
ha->pUserData->dwHeaderOffs - sizeof(TMPQUserData)
So the payload is now that span, clamped to the file because the redirect is
an untrusted u32, and both size fields are carried for inspection without
bounding anything. Using the capacity truncated an archive that reserved
more area than it filled; using it as the only bound returned the archive
itself as metadata when it was garbage.
The ambiguous field is renamed userDataHeaderSize to stop it reading like
the size of the 16-byte header, and format note 15 records the citation.
Sector CRC (P2-3),
(attributes)read + write (P2-4, #11), v2–v4 read via the classic tables (P2-2), user data header (P2-1), decoy-header resilience (P2-5b, #47).Notable: MPQ sector checksums are Adler-32 seeded with zero, so
java.util.zip.Adler32cannot be used. Caught bytools/mpqref.py, which now verifies the values independently in CI.HET/BET is split out as P2-2a and not attempted — not needed to extract StormLib-generated archives, and there is no fixture to verify it against. Recorded in AUDIT.md.
Spec decisions in
docs/mpq-format-notes.md§9–13.